Distribute cron tasks #3326
Open
Distribute cron tasks #3326
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tgregory-block
requested review from
frojasg,
adrw,
din-cashapp and
keeferrourke
| } | ||
|
|
||
| internal fun buildTaskLease(klass: KClass<out Runnable>): Lease { | ||
| val sanitizedClassName = klass.qualifiedName!!.replace(".", "-") |
There was a problem hiding this comment.
There are some gotchas here with inner/anonymous classes. You might end up with some very strange looking lease names. Is this replacement just for readability or are there actual characters/sequences you intend to avoid in the lease name string?
There was a problem hiding this comment.
We mainly weren't sure if the partition key had forbidden characters, and since the dynamo lease implementation lives in Skim it wasn't straightforward to write an integration test. Looking more thoroughly at the dynamo docs, it doesn't seem like that's the case though.
There was a problem hiding this comment.
For the anonymous object case, the existing implementation also force unwraps the runnable class's qualifiedName, so we're not breaking something that was previously possible.
There was a problem hiding this comment.
Removed the sanitization, . are clearly fine since the original cluster wide lease uses them
mhickman
reviewed
Jun 25, 2024
Comment on lines
+102
to
+103
| CronLeaseBehavior.ONE_LEASE_PER_CRON -> tryAcquireLeasePerCronAndRunCrons(lastRun) | ||
| CronLeaseBehavior.ONE_LEASE_PER_CLUSTER -> tryAcquireSingleLeaseAndRunCrons(lastRun) |
There was a problem hiding this comment.
worth adding any checks + behavior for rolling this out? migrating from per-cluster to per-cron means there'll likely be overlaps during a deploy
GitOrigin-RevId: 453df97428102c5f1959afd94c83d52623e8387c
GitOrigin-RevId: c51ec0fb5dcc67f9e1b6b2564678fd86d3f3c416
GitOrigin-RevId: 611be8a7a60a1decf8ea787aca23dacf01c1224a
GitOrigin-RevId: b6a255f455c40ef5e21059ab5582a939be572b35
implementation of `ClusterResourceMapper` from `murmur3_32` in the `com.google.common.hash.Hashing` package, to `murmur3_32_fixed`, which addresses a bug when generating a hashed value from a string containing non-BMP characters. GitOrigin-RevId: f1d4efcd61eb40270b838221bf8a0fc1bca38b1d
GitOrigin-RevId: 43385fdc20d5036dd27002569f988ad76328c3e0
from https://observablehq.com/@d3/mobile-patent-suits?intent=fork GitOrigin-RevId: 02456e0997b5e0a5c4c5dd2229edc0326bdabfc1
GitOrigin-RevId: 294c578de959770de4a3b1de8d99c5b122b85984
returns a misk class rather than inheriting one that returns a wisp class. Previously, if someone with a misk.feature.Attribute called `.with`, they would get back a wisp class which would be incompatible with the rest of the misk.feature.* classes they were using. GitOrigin-RevId: 7c5181263816f14e27fdaf36b4287840119d5ce4
services are shutdown by default GitOrigin-RevId: bce36d20af9ef0221f5179e4c7382854e3d7e04d
Currently, the [RedisModule](https://github.com/cashapp/misk/blob/8711bf7a6f68ed7b31237d37d811649d958486f2/misk-redis/src/main/kotlin/misk/redis/RedisModule.kt#L74) in Misk uses a [RedisConfig](https://github.com/cashapp/misk/blob/bdb3d56f8a418fa630433a6a8e374405c5a21f1e/misk-redis/src/main/kotlin/misk/redis/RedisConfig.kt#L7) map where the Jedis client is initialized using the first item in the map. The map is ordered by insertion which is based on the order of how configurations are loaded. This setup can lead to unintended consequences, particularly when multiple Redis clusters are provisioned for a single environment. This adds a new constructor in `RedisModule` and `RedisClusterModule` to improve config management by allowing consumers to provide a specific config to use instead of letting the module choose from a given configuration map. GitOrigin-RevId: 5175feb11d51f78f77b723dee9943741bf59b8fd
opinionated. We've reconsidered this approach and simplified it to just a function call `withSmartTags` doing away with the stateful builders etc. COPYBARA_INTEGRATE_REVIEW=#3366 from GitOrigin-RevId: 508c16fd4046947e33c3e282533841978ef2d1a8
query parameters are extracted. Advantage of this approach is that tests can pass the full url with queryparameters (e.g /get?param=value). Disadvantage is that this changes how we were constructing the url and there is a chance for breaking old behavior. GitOrigin-RevId: c5a36bd86576979ac1840f36e6ace3a1c130ed25
GitOrigin-RevId: efb52773373ea3687903bc3fb2825abc36c595bd
GitOrigin-RevId: 2d221e51cf17596ec6849d0f2dd4d015df83fe87
arguments GitOrigin-RevId: 05141a51cfaa6cb608383b5aa7d163b72347b114
function, so we can just implement the interface directly GitOrigin-RevId: e52510622590f1068de635b97cc73fc4e7a03dac
GitOrigin-RevId: 5c531499ac0f3028ff74d4efbbfceb1344ea7b46
GitOrigin-RevId: 22e9c9a4c3bb23f3874d7a1a8dac1bb37df5443d
GitOrigin-RevId: 739c0b99ae9c919d193c972af82e31e23385f04e
GitOrigin-RevId: 509e8755c63c32910d74beab22ecaacd6935d5fa
fixture GitOrigin-RevId: 8164ac1d5ed2ce86690a377921b05873aedf4a4e
GitOrigin-RevId: 489b1ed32a14f269a03f2089c9f577679b46be7c
GitOrigin-RevId: 23e05e36ea6d3f06c66d25b51b2af5f4eea4c65b
GitOrigin-RevId: 034d8309a3b9b7d471f900134a51f077093ef74c
default flush interval to allow configuring how often events are sent GitOrigin-RevId: 5e36390bc9fe80da834a7b8baf036591e85536fa
tgregory-block
force-pushed
the
tgregory/distribute-cron-tasks
branch
from
0461ab6
to
6d42628
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently Cron tasks are governed by a single lease, so all tasks are executed on whichever pod takes that lease. This change adds a new
CronLeaseBehaviorparameter toCronModule, making it possible to take out a lease per-cron-task and distribute the tasks across the deployment. The default behavior remains to take out a single lease